Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

src/openhuman/voice/always_on.rs (1)

598-642: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated anchor-matching logic between wake_word_present and extract_command.

Both functions independently recompute the same anchor selection (max_by_key on token length) and fuzzy-match scan (max_dist formula, 0..t.len().min(3) window). They currently agree, but any future tuning of the fuzzy-match thresholds in one function without the other will silently desync bare-wake acknowledgement from actual wake-word detection.

Consider factoring the shared scan into a helper returning the matched index (e.g. fn find_wake_anchor(transcript_tokens: &[String], wake_word: &str) -> Option<usize>), with extract_command and wake_word_present both calling it.

♻️ Suggested consolidation

-pub(crate) fn wake_word_present(transcript: &str, wake_word: &str) -> bool {
-    let wake = wake_tokens(wake_word);
-    if wake.is_empty() {
-        return false;
-    }
-    let t = wake_tokens(transcript);
-    let anchor = wake.iter().max_by_key(|w| w.len()).cloned().unwrap();
-    let max_dist = if anchor.chars().count() <= 4 { 1 } else { 2 };
-    (0..t.len().min(3)).any(|i| levenshtein(&t[i], &anchor) <= max_dist)
-}
+fn find_wake_anchor_index(t: &[String], wake: &[String]) -> Option<usize> {
+    if wake.is_empty() {
+        return None;
+    }
+    let anchor = wake.iter().max_by_key(|w| w.len()).cloned().unwrap();
+    let max_dist = if anchor.chars().count() <= 4 { 1 } else { 2 };
+    (0..t.len().min(3)).find(|&i| levenshtein(&t[i], &anchor) <= max_dist)
+}
+
+pub(crate) fn wake_word_present(transcript: &str, wake_word: &str) -> bool {
+    let wake = wake_tokens(wake_word);
+    let t = wake_tokens(transcript);
+    find_wake_anchor_index(&t, &wake).is_some()
+}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/voice/always_on.rs` around lines 598 - 642, `wake_word_present`
and `extract_command` duplicate the same wake-anchor selection and fuzzy scan
logic, which can drift over time. Factor the shared matching into a helper in
`always_on.rs` (for example, a function that finds the matched token index from
the transcript tokens and wake word), then have both `wake_word_present` and
`extract_command` call it so the anchor choice, distance threshold, and
first-3-token window stay consistent.

src/openhuman/config/ops/ui.rs (1)

277-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Trim only applied via this apply path.

reply_display_name is trimmed here on apply_meet_settings, and the downstream fallback in calendar.rs also .trim()s before use, so double-trimming is harmless. No length/content validation is imposed on the name (arbitrary length strings are persisted to config and reused verbatim as respondToParticipant), but since this is a user-set self-identifying label with no logging of its value shown in this diff, it's a low-priority concern.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/config/ops/ui.rs` around lines 277 - 282, The handling of
reply_display_name is normalized only in apply_meet_settings, while calendar.rs
trims again before use, so make the normalization rule consistent in one place.
Update apply_meet_settings and the downstream fallback that reads
config.meet.reply_display_name so the same trimming behavior is applied
uniformly across all paths that set or consume this field, using the existing
apply_meet_settings and reply_display_name symbols to keep the logic easy to
locate.

src/openhuman/agent_meetings/upcoming.rs (2)

36-58: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Errors on a connected calendar are silently swallowed to an empty list.

fetch_recall_upcoming degrades to Ok(Vec::new()) for any error from fetch_recall_meetings, not just "not connected." Contrast with the Composio branch a few lines below, which explicitly propagates per-connection API errors because "a failed list_connections is not the same as 'no connections'... it means we could not determine the connection state." For Recall, a real backend failure on an actively connected calendar (auth expiry, timeout, malformed response) will now render as "no upcoming meetings" instead of surfacing an error state to the UI.

This mirrors the same swallow-to-empty pattern in heartbeat::planner::collectors::collect_recall_calendar_meetings, so it's a deliberate, consistent choice — but it's worth confirming this UX tradeoff is intentional given the explicit error-propagation policy documented for the Composio path in this same file.

💡 Possible approach: distinguish "not connected" from other failures

     let meetings = match crate::openhuman::recall_calendar::ops::fetch_recall_meetings(config).await
     {
         Ok(m) => m,
-        Err(e) => {
+        Err(e) if is_not_connected_error(&e) => {
             tracing::info!(error = %e, "[meet:upcoming] recall calendar unavailable — skipping");
             return Ok(Vec::new());
         }
+        Err(e) => {
+            tracing::warn!(error = %e, "[meet:upcoming] recall meetings fetch failed");
+            return Err(e);
+        }
     };

This requires the backend/IntegrationClient error to carry a distinguishable "not connected" signal.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/agent_meetings/upcoming.rs` around lines 36 - 58,
`fetch_recall_upcoming` currently converts every `fetch_recall_meetings` failure
into `Ok(Vec::new())`, which hides real backend/auth/timeout errors as “no
meetings.” Update this function to distinguish the “not connected” case from
other errors, and only return an empty list for the former while propagating
genuine failures upward; use the existing `fetch_recall_meetings` call and
`build_recall_upcoming` flow as the place to add the error handling.

---

107-131: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Recall routing assumes connected when selected, and adds a status round-trip for everyone else.

Two related observations:

1. When recall_selected is true, recall_connected is forced to true without checking is_connected (i.e., without verifying the backend's enabled/connected flags). If the user picked Recall but the backend feature is disabled or they never completed OAuth, this still calls into fetch_recall_upcoming, which will fail and silently degrade to empty (see prior comment) instead of falling back to Composio.
2. When Recall is not selected, every call to fetch_upcoming_meetings now performs an extra is_connected → status() network round-trip before falling back to Composio. For the majority of users who never touch Recall, this adds latency to every meetings-page poll (and, per the heartbeat collectors.rs snippet, the same round-trip is duplicated on the heartbeat tick path too).

Worth considering a short-TTL cache for the Recall status check shared across the meetings-page and heartbeat paths to avoid the redundant per-poll network hit.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/agent_meetings/upcoming.rs` around lines 107 - 131, The Recall
routing in fetch_upcoming_meetings currently treats recall_selected as connected
and always does a live is_connected check for everyone else, which can both
misroute users and add repeated network latency. Update the upcoming.rs flow so
it only chooses fetch_recall_upcoming when the Recall backend is actually
enabled/connected, and otherwise cleanly falls back to Composio; keep the
decision logic tied to the existing recall_selected, recall_connected, and
is_connected paths. Also introduce a short-TTL shared cache for Recall status so
both the meetings-page and heartbeat callers can reuse the result instead of
paying the status() round-trip on every poll.

app/src/lib/recallCalendar/recallCalendarApi.ts (1)

11-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider extracting the shared CLI-envelope unwrap helper.

This duplicates the same { result, logs } unwrapping logic already implemented in lib/composio/composioApi.ts (per the comment). Extracting a shared unwrapCliEnvelope util would avoid divergence as both evolve.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/lib/recallCalendar/recallCalendarApi.ts` around lines 11 - 28, The
`unwrapCliEnvelope` logic in `recallCalendarApi.ts` is duplicated from the
existing helper in `lib/composio/composioApi.ts`; extract this `{ result, logs
}` unwrapping into a shared utility and have both `unwrapCliEnvelope` call sites
reuse it. Keep the shared helper generic, preserve the current envelope checks,
and update the recall calendar API to import and use the shared function instead
of maintaining a second copy.

app/src/lib/recallCalendar/hooks.test.ts (1)

14-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider covering the disconnect and error-recovery paths.

Only the "already connected on first poll" scenario is tested. Given the stale-error bug flagged in hooks.ts, a test asserting that a transient status() failure followed by a successful poll clears the error would have caught it, and a disconnect-flow test would improve confidence in the revert-to-composio path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/lib/recallCalendar/hooks.test.ts` around lines 14 - 40, Add coverage
in useRecallCalendar for the missing disconnect and error-recovery flows. Extend
the hook tests around recallCalendarApi.status to simulate a transient failure
followed by a successful poll and assert the hook clears the stale error state,
then add a disconnect-path test that verifies it reverts provider settings back
to composio and related state updates happen through
openhumanUpdateMeetSettings. Focus the tests on the useRecallCalendar hook
behavior rather than implementation details.

app/src/lib/recallCalendar/hooks.ts (1)

34-34: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Unconditional provider-flip write on first mount.

syncedProvider starts as null, so the very first successful poll always differs from desiredProvider and triggers an openhumanUpdateMeetSettings write even if the backend's calendar_provider already matches. This is a harmless-but-needless RPC on every mount since the hook never reads the current calendar_provider from status()/get_meet_settings before deciding to flip.

Also applies to: 40-51

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/lib/recallCalendar/hooks.ts` at line 34, The `useRecallCalendarSync`
hook is writing an unnecessary provider flip on first mount because
`syncedProvider` starts as `null` and always differs from `desiredProvider`.
Update the initial sync logic around `syncedProvider`, `status()`, and
`openhumanUpdateMeetSettings` so the hook first learns the current
`calendar_provider` from the backend (via `status()`/`get_meet_settings`) before
deciding whether to write. Only call the update when the backend value actually
differs from the desired provider, and initialize `syncedProvider` from the real
current provider to avoid the first-poll RPC.

app/src/components/meetings/MeetDefaultsDrawer.tsx (1)

197-204: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Skip the redundant save when the field is blurred without changes.

handleReplyDisplayNameBlur always calls persist(...) on blur, even if the user didn't edit the field (e.g., tab-through or click-and-away). This fires an unnecessary RPC call and flashes the saving/"Saved" status line every time, unlike the other handlers here which are wired to onChange and only fire on an actual value change.

♻️ Proposed fix to skip no-op saves

+  // Tracks the last value confirmed by the backend (initial load or a
+  // successful save) so blur doesn't re-persist when nothing changed.
+  const lastSavedReplyDisplayNameRef = useRef('');
+
   // Persist the display name on blur (not per keystroke). Trim before saving so
   // the anchor match is clean; skip the write when nothing changed.
   const handleReplyDisplayNameBlur = () => {
     const trimmed = replyDisplayName.trim();
     if (trimmed !== replyDisplayName) setReplyDisplayName(trimmed);
+    if (trimmed === lastSavedReplyDisplayNameRef.current) return;
+    lastSavedReplyDisplayNameRef.current = trimmed;
     void persist('reply_display_name', { reply_display_name: trimmed });
   };

And set lastSavedReplyDisplayNameRef.current = s.reply_display_name ?? ''; alongside setReplyDisplayName(s.reply_display_name ?? ''); in the initial load effect.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/components/meetings/MeetDefaultsDrawer.tsx` around lines 197 - 204,
Skip the no-op save in handleReplyDisplayNameBlur: only call
persist('reply_display_name', ...) when the trimmed value differs from the last
saved reply display name, and keep the state in sync after trimming. Also update
the initial load effect that sets replyDisplayName so
lastSavedReplyDisplayNameRef.current is initialized from s.reply_display_name,
ensuring blur without edits does not trigger an unnecessary RPC or status flash.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes